Skip to content

Conversation

@mrober
Copy link
Contributor

@mrober mrober commented Mar 17, 2025

Simplify settings package and a few more classes using DI.

The biggest example is SessionsSettings, which used to have multiple secondary constructors. This
change lets us avoid plumbing dependencies like DataStore<> through multiple classes.

Moved DataStore construction into provides, which will be needed for MultiProcessDataStoreFactory.

Also cleaned up tests a bit. We shouldn't use Dagger in the unit tests on Android. Once we remove
the bound service, it will be easy to remove DI from the unit tests. Instead, unit tests will just
call the constructors directly and pass in real or fake instances.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 17, 2025

📝 PRs merging into main branch

Our main branch should always be in a releasable state. If you are working on a larger change, or if you don't want this change to see the light of the day just yet, consider using a feature branch first, and only merge into the main branch when the code complete and ready to be released.

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Mar 17, 2025

Coverage Report 1

Affected Products

  • firebase-sessions

    Overall coverage changed from 71.35% (76990d9) to 66.63% (54f8a09) by -4.72%.

    18 individual files with coverage change

    FilenameBase (76990d9)Merge (54f8a09)Diff
    FirebaseSessionsComponent_MainModule_Companion_ApplicationInfoFactory.java?0.00%?
    FirebaseSessionsComponent_MainModule_Companion_SessionConfigsDataStoreFactory.java?0.00%?
    FirebaseSessionsComponent_MainModule_Companion_SessionDetailsDataStoreFactory.java?0.00%?
    FirebaseSessionsComponent_MainModule_Companion_TimeProviderFactory.java?0.00%?
    FirebaseSessionsComponent_MainModule_Companion_UuidGeneratorFactory.java?0.00%?
    LocalOverrideSettings_Factory.java?0.00%?
    RemoteSettings.kt88.41%88.73%+0.33%
    RemoteSettingsFetcher.kt68.29%0.00%-68.29%
    RemoteSettingsFetcher_Factory.java?0.00%?
    RemoteSettings_Factory.java?0.00%?
    SessionDatastore.kt2.70%3.33%+0.63%
    SessionGenerator.kt92.00%90.91%-1.09%
    SessionGenerator_Factory.java?0.00%?
    SessionsSettings.kt94.74%96.88%+2.14%
    SettingsCache.kt90.48%93.55%+3.07%
    SettingsCache_Factory.java?0.00%?
    TimeProvider.kt50.00%0.00%-50.00%
    UuidGenerator.kt?0.00%?

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/SKvCwsmcK8.html

@github-actions
Copy link
Contributor

github-actions bot commented Mar 17, 2025

Test Results

 38 files   -    994   38 suites   - 994   55s ⏱️ - 33m 27s
 94 tests  -  5 768   94 ✅  -  5 746  0 💤  - 22  0 ❌ ±0 
188 runs   - 11 599  188 ✅  - 11 555  0 💤  - 44  0 ❌ ±0 

Results for commit 617945b. ± Comparison against base commit 76990d9.

This pull request removes 5768 tests.
com.google.android.datatransport.cct.CctBackendFactoryTest ‑ create_returnCCTBackend_WhenBackendNameIsCCT
com.google.android.datatransport.cct.CctDestinationTest ‑ cctDestination_shouldOnlySupportProtoAndJson
com.google.android.datatransport.cct.CctDestinationTest ‑ cctDestination_shouldSupportProtoAndJson
com.google.android.datatransport.cct.CctTransportBackendTest ‑ decorate_whenOffline_shouldProperlyPopulateNetworkInfo
com.google.android.datatransport.cct.CctTransportBackendTest ‑ decorate_whenOnline_shouldProperlyPopulateNetworkInfo
com.google.android.datatransport.cct.CctTransportBackendTest ‑ schedule_shouldAddCookieOnPseudonymousIds
com.google.android.datatransport.cct.CctTransportBackendTest ‑ schedule_shouldDropCookieOnMixedPseudonymousIds
com.google.android.datatransport.cct.CctTransportBackendTest ‑ send_CompressedResponseIsUncompressed
com.google.android.datatransport.cct.CctTransportBackendTest ‑ send_whenBackendRedirectsMoreThan5Times_shouldOnlyRedirect4Times
com.google.android.datatransport.cct.CctTransportBackendTest ‑ send_whenBackendRedirects_shouldCorrectlyFollowTheRedirectViaPost
…

♻️ This comment has been updated with latest results.

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Mar 17, 2025

Size Report 1

Affected Products

  • firebase-sessions

    TypeBase (76990d9)Merge (54f8a09)Diff
    aar192 kB203 kB+10.6 kB (+5.5%)
    apk (aggressive)378 kB644 kB+267 kB (+70.7%)
    apk (release)6.31 MB6.31 MB+6.52 kB (+0.1%)

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/Fe7GiuR4EG.html

Copy link
Contributor

@themiswang themiswang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's verify with the release plan before merge

Copy link
Contributor

@tejasd tejasd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - but there's still some RemoteSettingsTest and SessionSettingsTest failing which we should probably fix.

@mrober mrober merged commit f86d40d into main Mar 19, 2025
36 checks passed
@mrober mrober deleted the mrober/settingsdi branch March 19, 2025 13:24
tejasd pushed a commit that referenced this pull request Apr 1, 2025
Simplify settings package and a few more classes using DI.

The biggest example is `SessionsSettings`, which used to have multiple
secondary constructors. This
change lets us avoid plumbing dependencies like `DataStore<>` through
multiple classes.

Moved DataStore construction into provides, which will be needed for
`MultiProcessDataStoreFactory`.

Also cleaned up tests a bit. We shouldn't use Dagger in the unit tests
on Android. Once we remove
the bound service, it will be easy to remove DI from the unit tests.
Instead, unit tests will just
call the constructors directly and pass in real or fake instances.
@firebase firebase locked and limited conversation to collaborators Apr 19, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants